-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX / DOC Add support ElementalArea in DataObject #978
FIX / DOC Add support ElementalArea in DataObject #978
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasted able to get the CMS preview to work. Using the debugger, cmsPreview() nor PreviewLink were getting called. I followed the instructions inthe document I'll paste my code below.
Also could you rename |
5f92a13
to
cbced82
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like these docs are duplication of what's already on framework - https://docs.silverstripe.org/en/4/developer_guides/customising_the_admin_interface/preview/ / https://github.com/silverstripe/silverstripe-framework/blob/4/docs/en/02_Developer_Guides/15_Customising_the_Admin_Interface/04_Preview.md
We should aim to not have duplicated docs if possible - would it be possible to just remove the duplicated bits and link to the framework docs?
cbced82
to
4c74c46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems a lot better now that the Previewable examples have been removed.
I've followed the example code locally, the BlogPost's aren't publishable, which is unexpected given the content blocks are inline publishable
1ea26f7
to
693a6d9
Compare
0d5f0be
to
e6809e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't actually tried running the example locally yet, but I figured I may as well submit this much of the review while I do that.
There are some questions here that you may have already gone through with Steve - if so, sorry about the duplication. Just want to make sure I fully understand what is happening here.
c4cb739
to
28c171e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
There are a few minor tweaks to make on the documentation, and some code quality tweaks to make, but I think it's just about ready.
I haven't done so in my review, but once you've made these changes can you please make sure to run phpcs
? (You probably already do this but just in case)
At some stage we'll want behat coverage to ensure elemental blocks continue to work in arbitrary DataObject
s in the future... I'll let you decide whether you want to do that with this PR or create an issue for it to be added later, given the time you've already spent on this one.
After thinking about this I'm taking this decision for you :p just creat an issue, it should be a separate piece of work. Adding behat tests for this will be a big piece of work.
Regarding "After thinking about this I'm taking this decision for you :p just creat an issue, it should be a separate piece of work. Adding behat tests for this will be a big piece of work." I also want to add test cases for canDelete, Link and AbsoluteLink methods. |
You'd also need to add fixtures for it, which can't be done easily as
That sounds like a good idea |
590ad9e
to
b488f38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing. Ignore the code coverage one for now - there's not really anything you can do about that. I'm hoping it will resolve itself by the time you push a fix for the other failing tests.
6258888
to
18a09ae
Compare
There's a note above to address, plus some changes requested in silverstripe/silverstripe-frameworktest#105 - once those are addressed I'll test the behat tests locally |
18a09ae
to
38a50b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, should have noticed this in the last review.
Looks good now, I'll check that the behat tests run with the fixtures in frameworktest when I have a moment. If that passes fine, I'll approve the fixtures and then get travis to run the tests again here before merging.
38a50b0
to
e9cf576
Compare
Behat tests work locally. Restarting travis tests now that the fixtures are merged. |
e9cf576
to
d867213
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are failing that aren't failing on the 4.8
branch
ae97360
to
1306ae4
Compare
…f using ElemenatalArea with DataObject
1306ae4
to
d117559
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested locally, works as expected.
The only remaining failures are also observed on the 4.8 branch, and are unrelated to this PR.
Description
Create new section in docs directory with example how to use ElementalArea together with DataObject.
Changes
New condition was added in CMSEditLink() in BaseElement class for cause when $page is not an instance of SiteTree and Element is not inline editable. Also new condition for checking of existing the CMSEditLink() method in class that has relation with ElemantalArea.
Parent Issue